Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize AIActivationService from preferences #15044

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mvtec-richter
Copy link
Contributor

What it does

In our Theia-derived IDE, the AI feature sometimes is not enabled properly, even though the setting is enabled. We traced this down to the AIActivationServer depending on a signal from the PreferenceService.

Fixes #15043

How to test

We could not reproduce this with TheiaIDE, only with our Theia-derived IDE on Windows/electron. If you think, the proposed change should not be necessary, I will try to reduce our IDE to a minimal example.

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Contributed by MVTec Software GmbH

Review checklist

Reminder for reviewers

In our Theia-derived IDE, the AI feature sometimes is not enabled
properly, even though the setting is enabled. We traced this down
to the AIActivationServer depending on a signal from the
PreferenceService.

Fixes eclipse-theia#15043

Signed-off-by: Florian Richter <[email protected]>
@@ -44,7 +44,8 @@ export class AIActivationService implements FrontendApplicationContribution {
return this.isAiEnabledKey.get() ?? false;
}

initialize(): MaybePromise<void> {
async initialize(): Promise<void> {
await this.preferenceService.ready;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdirix Is this alright from a performance perspective, or should we rather use preferenceService.ready.then(() => ...)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should never await something which does not need to be awaited in the initialize startup calls. Therefore I would prefer the ready.then variant. I also don't see a harm for this case. Good catch!

@sdirix
Copy link
Member

sdirix commented Feb 25, 2025

Is there an explanation why the original code fails for the reported Theia based application but does not fail for the Theia IDE?

From my understanding the existing code should work as for every non-default preference a change event should be sent when the preferences are loaded. This feels like fixing a symptom while missing some underlying root cause.

Edit: Is it possibly a race condition between the preference service loading and the startup of the ai package, i.e. there is no guarantee that we catch all events?

@mvtec-richter
Copy link
Contributor Author

I further investigated this. I think, this happens, because we have a "license check". We overwrite FrontendApplication.start and halt startup until a process that is started by the backend confirmed, that the user has a valid license for our product.
In the mean time, the PreferenceService seems to become ready.

As I understand now, the FrontendApplicationContribution.initialize function are treated really time sensitive, but I'm still not sure, if this guarantees, that PreferenceService does not initialize faster.
I checked different locations, where FrontendContribution.initialize uses preferences:

I still think, when attaching to the PreferenceChangedEvent, the current state should be retrieved as well. But as FrontendApplicationContribution.initialize is time sensitive, we should not wait until the service becomes ready.

We still might need to rethink how to do our license check.

@sdirix
Copy link
Member

sdirix commented Feb 27, 2025

@mvtec-richter thanks for the additional context. I think the suggestion by @msujew is the best of both worlds, so if you adapt the code to:

protected updateEnableValue(enable: boolean): void {
  if(enable !== this.isAiEnabledKey.get()){
        this.isAiEnabledKey.set(enableValue);
        this.onDidChangeAIEnabled.fire(enableValue);
   }
}

initialize(): MaybePromise<void>{

  this.isAiEnabledKey = this.contextKeyService.createKey(ENABLE_AI_CONTEXT_KEY, false);
  // make sure we don't miss once preferences are ready
  this.preferenceService.ready.then(() => {
     const enableValue = this.preferenceService.get<boolean>(PREFERENCE_NAME_ENABLE_AI);
     this.updateEnableValue(enableValue);
  }

  this.preferenceService.onPreferenceChanged(e => {
      if (e.preferenceName === PREFERENCE_NAME_ENABLE_AI) {
          this.updateEnableValue(e.newValue);
      }
  });
}

Then this should improve the current situation without any performance issues. Note that I did just type this here into the GH issue, so there might be some issues with the code when copied as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

After restart AI feature is disabled, although the setting says otherwise
3 participants